-
Notifications
You must be signed in to change notification settings - Fork 357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(Wizard - next): supporting component unit tests #7731
feat(Wizard - next): supporting component unit tests #7731
Conversation
Preview: https://patternfly-react-pr-7731.surge.sh A11y report: https://patternfly-react-pr-7731-a11y.surge.sh |
057f828
to
8eb62e9
Compare
8eb62e9
to
8f07d76
Compare
8f07d76
to
3eb5f6c
Compare
/** Navigate using the step index */ | ||
goToStepByIndex: (index: number) => void; | ||
/** The button's aria-label */ | ||
/** Custom WizardNav or callback used to create a default WizardNav */ | ||
nav?: DefaultWizardNavProps | CustomWizardNavFunction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible/wise to have the nav and the footer both use the same pattern? Right now, the custom footer is a props object or a react node and the nav is a props object or a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary, but we could make a CustomWizardFooterFunction
if you want that returns onNext
, onBack
, and onClose
. Thoughts on that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd be helpful if people want a custom footer with custom buttons, right? so maybe that'd be wise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. That will make things complicated with how the footer is managed in context. We'd end up initializing footer's state variable as a function, which I'm not sure is even possible. Maybe I can try to do this in the next PR here where I'm updating a bunch of types (and including a new one called CustomWizardNavItemFunction
- very similar to CustomWizardNavFunction
):
https://github.com/patternfly/patternfly-react/pull/7915/files#diff-087f56845137e426d26fe8b189b155902a59de75ee7e2beff5c07db4d4f9c25f
Unless you think this is a blocking change for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's blocking since all this is beta :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok cool. I will address it in that other PR then, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I won't have time to review this in depth until I return from PTO on Friday 9/9, but one thing that I know will need to change is that the userEvent
usage will need to be updated to be compatible with user-event v14+
3eb5f6c
to
0007e01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This has been addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a couple of nits and a couple of questions about places where I think it would be better to test behavior on a sub-component directly.
packages/react-core/src/next/components/Wizard/__tests__/Wizard.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/next/components/Wizard/__tests__/WizardBody.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/next/components/Wizard/__tests__/WizardToggle.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/next/components/Wizard/__tests__/WizardToggle.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
20a54bd
to
b36baeb
Compare
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #7737